Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add styles based on the documentation tool #473

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

humitos
Copy link
Member

@humitos humitos commented Dec 16, 2024

Use our heuristic to detect the documentation tool/theme and add specific --readthedocs-* CSS variables based on that for known tools/themes.

Reference: readthedocs/readthedocs.org#11849 (comment)

Use our heuristic to detect the documentation tool/theme and add specific
`--readthedocs-*` CSS variables based on that for known tools/themes.

Reference: readthedocs/readthedocs.org#11849 (comment)
@humitos humitos requested a review from a team as a code owner December 16, 2024 10:46
@humitos humitos requested a review from agjohnson December 16, 2024 10:46
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is a better pattern here for dynamic styles. The Lit documentation probably suggests using classMap on the element, which could be another option. We've talked about avoiding classes for styling though, so I mostly lean towards a dynamic styles getter if it works easily.

src/flyout.js Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Dec 17, 2024

I believe there is a better pattern here for dynamic styles. The Lit documentation probably suggests using classMap on the element, which could be another option

Yes. Is there are way to detect if the --readthedocs-* variables were defined by the user from JavaScript? The pattern that I want to implement here is:

  1. Check if the user defined --readthedocs-* CSS variables
  2. If those were defined, do nothing
  3. If not, add a mkdocs-material CSS class to the element that redefines --readthedocs-*

Would that make sense?

I mostly lean towards a dynamic styles getter if it works easily.

I'm not sure to understand what this means. Can you expand o this?

@humitos
Copy link
Member Author

humitos commented Dec 17, 2024

I pushed a commit to what I understood it could be a better pattern for this. Let me know what you think.

@agjohnson
Copy link
Contributor

I'm not sure to understand what this means. Can you expand o this?

Using a getter for styles property allows for conditional styles without adding class names.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get#using_getters_in_classes

class SomeElement extends LitElemeent {
  static get styles {
    if (...) {
      return css`...`;
    }
    return css`...`;
  }
}

Would that make sense?

So, there is getComputedStyle: https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle

But it feels like better design to make this explicit and rely on existing CSS functionality for this. If we keep our rules low specificity, then user added rules can very easily be higher specificity -- and our styles are then overridden.

If not, add a mkdocs-material CSS class to the element that redefines --readthedocs-*

Using class names is a separate pattern, but also possible.

In general, a pattern I like is to (re)use element properties for CSS rules instead of classes. Element properties have specific purpose and update the Lit element properties where class names don't do anything but affect CSS styles. That is:

// The `tool` property affects the element reactive properties. This rule is specificity 0 1 1
readthedocsflyout[tool="docusaurus"] { ... }
// The CSS class is used only for styles, it doesn't affect the properties on the Lit element. Specificity 0 1 1
readthedocsflyout.tool-docusaurus { ... }

We should keep specificity as low as possible either way.


One wrench I'll give you though is that this is still just for readthedocs-flyout, and we probably don't want to have each element independently guessing the tool and applying styles. I'm guessing your initial attempt was closer to what you wanted. What I pointed out as the problem applying styles to html/:root from inside FlyoutElement is that this affects styles of other elements, and likewise doesn't apply these styles if the user has the flyout disabled.

So I think the styles you applied originally might be closer to what we want, we just shouldn't do this from inside a single element. This is something we should do a more global level.

I might still suggest a class/attribute selector approach here though, which leverages CSS functionality instead of hiding this logic from user in JS.

:root[data-readthedocs-tool="docusaurus"] { ... }
:root[data-readthedocs-tool="sphinx"] { ... }

This gives a clear attribute users can remove and we can avoid applying an attribute if a user already specifies an attribute or something like data-readthedocs-tool="manual".

src/flyout.js Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Dec 18, 2024

One wrench I'll give you though is that this is still just for readthedocs-flyout, and we probably don't want to have each element independently guessing the tool and applying styles. I'm guessing your initial attempt was closer to what you wanted. What I pointed out as the problem applying styles to html/:root from inside FlyoutElement is that this affects styles of other elements, and likewise doesn't apply these styles if the user has the flyout disabled.

I was trying to keep things scoped by addon. That is, from flyout we should be defining only settings that affect the flyout (e.g.--readthedocs-flyout-* variables); without affecting other addons/elements.

Also, we can't do :root[data-readthedocs-tool="docusaurus] because that selector doesn't return anything since we are using shadow DOMs.

@humitos
Copy link
Member Author

humitos commented Dec 18, 2024

I pushed some changes that use tool=sphinx and tool-theme=furo attributes on readthedocs-flyout to define specific CSS rules based on the documentation tool and theme. I think I like this approach. Feedback welcome.

We will need to do the same for the other addons, where we we want to increase/decrease the font sizes. However, I'm happy to start with flyout for now and grow from there if we find this is the pattern we will stick to.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach with the CSS -- it feels really explicit which feels good.

@@ -31,9 +37,12 @@ export class FlyoutElement extends LitElement {
super();

this.config = null;
this.classes = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we're setting this, and then overriding it on line 44?

Comment on lines +324 to +325
tool="${docTool.documentationTool}"
tool-theme="${docTool.documentationTheme}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to prefix these with something? I guess we're controlling it, but I imagine we also want the CSS selectors prefixed, unless it's OK because it's in the shadow DOM?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we work with our custom elements, no prefixing is required. When working with native elements, data- prefix should be used.

I don't think we should use div here, or apply the tool/tool-theme attribute here though. I was describing something different with CSS control of this attribute

@agjohnson
Copy link
Contributor

Also, we can't do :root[data-readthedocs-tool="docusaurus] because that selector doesn't return anything since we are using shadow DOMs.

If you need to manage these variables inside each addon then this is true, but what I was describing was applying these variables from the host DOM, at a global level. This is where users should/will be defining our CSS variables either way.

I was trying to keep things scoped by addon. That is, from flyout we should be defining only settings that affect the flyout (e.g.--readthedocs-flyout-* variables); without affecting other addons/elements.

I do think isolation makes sense, but we can still do addon isolation of styles/variables at a global level. We don't need to apply --readthedocs-flyout-font-size to only readthedocs-flyout or just one instance of <readthedocs-flyout>, that variable can be applied to html and still affect <readthedocs-flyout> the same.

Thinking more, we don't need an attribute or class on html. It feels like our doc detection rules should happen at a addon configuration/initialization and add the following variables at html:

// Styles applied for docusaurus
html {
  --readthedocs-flyout-font-size: 0.9rem;
  --readthedocs-notification-font-size: 0.9rem;
}

The easiest change for theme maintainers and users will be to add some CSS like:

// `html body` or `html html` is higher specificity than our rules at `html`.
html body, html html {
  --readthedocs-flyout-font-size: 2rem;
  --readthedocs-notification-font-size: 2rem;
}

This doesn't involve editing the doc tool theme HTML, so is the easiest control point.

Defining these variables inside the shadow DOM like you're describing might take precedence over host applied variables though, we should test this. This would make it hard/impossible for this easy control of these variables.

This all gives users/theme maintainers some way to control at least the styles applied by our magic, but it doesn't control or allow opt out from our magic detection. Attributes on each addon does accomplish this but it's rather cumbersome -- maintainers would have to define all our addons in HTML theme <readthedocs-flyout tool="custom" /><readthedocs-notification tool="custom" />....

@humitos
Copy link
Member Author

humitos commented Dec 19, 2024

By using selectors like html { --readthedocs-flyout-font-size: 0.65rem }, I'm only able to modify things exposed by --readthedocs-* variables; which is not enough in all cases. For example, I need to modify line-height for some of the themes/doctools and I'm not being able to. I think we will end up having one --readthedocs-* variable per addon per property if we follow this pattern 😞 (we already have one --readthedocs-*-font-size per addon)

I think that using --readthedocs-* is getting pretty complicated to allow us improving the look and feel of the addons while also letting users to modify them. We added them as an initial test to allow ourselves to document how to adjust the sizes of the addons because we noticed they didn't render properly everywhere (mainly on Material for MkDocs). However, this PR implements the better look and feel/styling automatically without user intervention. I'd propose to revert the idea of these --readthedocs-* variables and give us full control back over the addons until we figure it out a more generic pattern for customization in #51.

References:

Until then, this PR will give users a better UI/UX anyways since they won't have to do anything on their side. In other words, I'd like to give users the best default experience on their doctools/themes (at least the ones we known) for now (the proposal of this PR) and leave a more polished customization for the future.

@ericholscher
Copy link
Member

I don't fully understand the implementation details here, but the goal I assume is having good defaults, but letting users be able to override them. If we have to choose only 1, I think good defaults is more important, since the initial experience for the user is most important, and getting that right is going to make adoption much more likely. However, it really seems like we should be able to support both, where we can set a default, and the user can override it?

@agjohnson
Copy link
Contributor

or example, I need to modify line-height for some of the themes/doctools and I'm not being able to.

Line height should almost always be derived from font-size though, as a proportional value to the height of the font (1.2em) or as a calculated value (calc())

I think we will end up having one --readthedocs-* variable per addon per property if we follow this pattern

What styles do we need that shouldn't be derived from font-size? This would be helpful to enumerate. The majority of styles seem like be positioning and spacing, both mostly proportional to font-size.

Even if there are many other variables required, I still think adding more variables is a good pattern. These variables don't need to map directly to CSS styles directly even.

CSS variables are a benefit because they avoid many of the downsides to JS and HTML customization. More control of CSS styles could render further customization needless even. But also important: a worst case failure of CSS customization is just improper display. JS and HTML customization can throw exceptions if we introduce changes that break user customization.

I'd propose to revert the idea of these --readthedocs-* variables and give us full control

How do we replace user customization for this PR then, what are users expected to do to override our magic?

So far I don't agree with removal, for the points above.

think that using --readthedocs-* is getting pretty complicated to allow us improving the look and feel of the addons while also letting users to modify them.

Agreed, I still think this is completely possible here too. Good defaults are helpful, but we really shouldn't be solely responsible for customization at every tool either. Retaining easy customization points shares this load and doesn't cut out theme maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants